🎉 Source Shopify: Add BalanceTransactions stream
#9850
Conversation
github-actions
bot
added
area/connectors
|
Hi @harshithmullapudi ! Sorry to bother you, but I just want to know if you need something else from my side to continue with the review of this PR. |
|
Hey we are just less in number of folks working so kinda crunch on time but
will review it today or tomorrow. Thanks for the patience. Will surely get
back to you if any need comes
|
|
Hey can you run sourceAcceptanceTests and share the screenshots? |
@harshithmullapudi , AcceptanceTests's output is present in the PR description, under |
|
Hey yeah I ran the tests and they were failing
|
|
hey looks like the way the balance stream is implemented its params are order _id. You can find more |
|
Hy! Acceptance tests for Maybe you are getting Acceptance tests
❯ docker pull airbyte/source-acceptance-test
Using default tag: latest
latest: Pulling from airbyte/source-acceptance-test
Digest: sha256:3431e847124f13a0a94a62c91de313f0e2a69c1419aa6946d0c58a8b5383dfe4
Status: Image is up to date for airbyte/source-acceptance-test:latest
docker.io/airbyte/source-acceptance-test:latest
❯ python -m pytest integration_tests -p integration_tests.acceptance
Test session starts (platform: linux, Python 3.7.9, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /home/da/f14/git/airbyte, configfile: pytest.ini
plugins: sugar-0.9.4, requests-mock-1.9.3, timeout-1.4.2
collecting ...
airbyte-integrations/connectors/source-shopify/integration_tests/integration_test.py::test_dummy_test ✓ 6% ▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_match_expected[inputs0] ✓ 11% █▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_required[inputs0] ✓ 17% █▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_optional[inputs0] ✓ 22% ██▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_has_secret[inputs0] ✓ 28% ██▊
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_secret_never_in_the_output[inputs0] ✓ 33% ███▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_defined_refs_exist_in_json_spec_file[inputs0] ✓ 39% ███▉
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_oauth_flow_parameters[inputs0] ✓ 44% ████▌
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs0] ✓ 50% █████
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs1] ✓ 56% █████▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs2] ✓ 61% ██████▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_discover[inputs0] ✓ 67% ██████▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_cursors_exist_in_schema[inputs0] ✓ 72% ███████▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_refs_exist_in_schema[inputs0] ✓ 78% ███████▊
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― TestBasicRead.test_read[inputs0] ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
self = <source_acceptance_test.tests.test_core.TestBasicRead object at 0x7f1ca70bcc10>, connector_config = SecretDict(******)
configured_catalog = ConfiguredAirbyteCatalog(streams=[ConfiguredAirbyteStream(stream=AirbyteStream(name='customers', json_schema={'type': ... 'incremental'>, cursor_field=['id'], destination_sync_mode=<DestinationSyncMode.append: 'append'>, primary_key=None)])
inputs = BasicReadTestConfig(config_path='secrets/config.json', configured_catalog_path='integration_tests/configured_catalog.j...s={'abandoned_checkouts'}, expect_records=None, validate_schema=True, validate_data_points=False, timeout_seconds=3600)
expected_records = [], docker_runner = <source_acceptance_test.utils.connector_runner.ConnectorRunner object at 0x7f1ca6f75e50>
detailed_logger = <Logger detailed_logger acceptance_tests_logs/test_core.py__TestBasicRead__test_read[inputs0].txt (DEBUG)>
def test_read(
self,
connector_config,
configured_catalog,
inputs: BasicReadTestConfig,
expected_records: List[AirbyteMessage],
docker_runner: ConnectorRunner,
detailed_logger,
):
output = docker_runner.call_read(connector_config, configured_catalog)
records = [message.record for message in filter_output(output, Type.RECORD)]
assert records, "At least one record should be read using provided catalog"
if inputs.validate_schema:
self._validate_schema(records=records, configured_catalog=configured_catalog)
> self._validate_empty_streams(records=records, configured_catalog=configured_catalog, allowed_empty_streams=inputs.empty_streams)
../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:327:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <source_acceptance_test.tests.test_core.TestBasicRead object at 0x7f1ca70bcc10>
records = [AirbyteRecordMessage(stream='collects', data={'id': 19149737197733, 'collection_id': 218563281061, 'product_id': 5630...', 'shop_url': '****', 'id': '54676127909|38210170323109'}, emitted_at=1644870000607, namespace=None), ...]
configured_catalog = ConfiguredAirbyteCatalog(streams=[ConfiguredAirbyteStream(stream=AirbyteStream(name='customers', json_schema={'type': ... 'incremental'>, cursor_field=['id'], destination_sync_mode=<DestinationSyncMode.append: 'append'>, primary_key=None)])
allowed_empty_streams = {'abandoned_checkouts'}
def _validate_empty_streams(self, records, configured_catalog, allowed_empty_streams):
"""
Only certain streams allowed to be empty
"""
counter = Counter(record.stream for record in records)
all_streams = set(stream.stream.name for stream in configured_catalog.streams)
streams_with_records = set(counter.keys())
streams_without_records = all_streams - streams_with_records
streams_without_records = streams_without_records - allowed_empty_streams
> assert not streams_without_records, f"All streams should return some records, streams without records: {streams_without_records}"
E AssertionError: All streams should return some records, streams without records: {'order_risks', 'price_rules', 'order_refunds', 'products', 'customers', 'balance_transactions', 'metafields', 'fulfillments', 'pages', 'transactions', 'discount_codes', 'orders', 'draft_orders', 'fulfillment_orders', 'inventory_items', 'custom_collections'}
E assert not {'balance_transactions', 'custom_collections', 'customers', 'discount_codes', 'draft_orders', 'fulfillment_orders', ...}
../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:244: AssertionError
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestBasicRead.test_read[inputs0] ⨯ 83% ████████▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_full_refresh.py::TestFullRefresh.test_sequential_reads[inputs0] ✓ 89% ████████▉
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_two_sequential_reads[inputs0] ✓ 94% █████████▌
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_state_with_abnormally_large_values[inputs0] ✓ 100% ██████████
========================================================================================================== short test summary info ===========================================================================================================
FAILED ../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestBasicRead::test_read[inputs0] - AssertionError: All streams should return some records, streams without records: {'order_risks', 'price_rules', 'o...
Results (142.10s):
17 passed
1 failed
- airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:310 TestBasicRead.test_read[inputs0]
|
|
@ron-damon we're waiting our team add data to validate the final part of integration tests. |
There was a problem hiding this comment.
Looks fine, but this implementation is not complete.
It misses the Payouts stream, which is needed in order to fetch the BalanceTransactions stream, see the docs: https://shopify.dev/api/admin-rest/2022-01/resources/transactions#resource-description
We need to implement the Payouts stream to have the payout_id and use it inside the BalaceTransactions stream API calls.
What can be done here at this point:
- The contributor just adds the
Payoutsstream and makes it work withBalanceTransactionsstream usingstream_slicesmethod. - We will leverage this contribution in order to complete it by ourselves, refactor this implementation and finish it by covering all the implementation and all the tests on our end.
@ron-damon @harshithmullapudi WDYT?
|
Hey @bazarnov, thanks for taking a look. According to the documentation, the IMHO, it's better to have this stream independent from another stream ( Make sense to you? |
Got your point. It's reasonable having the After that, please edit the This means If you faced with the issue of missing This will make it pass. If you have any questions, feel free to ask here. |
|
I think we will continue this in #10204 |
|
I think we can close this PR, due to this one |
What
Adds
BalanceTransactionsstream to Shopify source connector.How
idlogic in a new classIncrementalByIDShopifyStreamthat inherits fromIncrementalShopifyStreamCollectsto inherit fromIncrementalByIDShopifyStreamBalanceTransactionsRecommended reading order
source_shopify/source.py🚨 User Impact 🚨
None.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/SUMMARY.mddocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampledocs/integrations/README.mdairbyte-integrations/builds.mdAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing./publishcommand described hereUpdating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing./publishcommand described hereConnector Generator
-scaffoldin their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplatesthen checking in your changesTests
Unit
Integration
❯ python -m pytest integration_tests Test session starts (platform: linux, Python 3.7.9, pytest 6.2.5, pytest-sugar 0.9.4) cachedir: .pytest_cache rootdir: /home/da/f14/git/airbyte, configfile: pytest.ini plugins: sugar-0.9.4, requests-mock-1.9.3, timeout-1.4.2 collecting ... airbyte-integrations/connectors/source-shopify/integration_tests/integration_test.py::test_dummy_test ✓ 100% ██████████ Results (0.02s): 1 passedAcceptance
Acceptance tests for
secrets/config_oauth.jsonwere disabled because we only have API Password.There is 1 test failing because the account used for them doesn't have data for many streams. We've other shops that have a lot of data, but the tests take too much time to fetch all the data.